Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Adds precompiled headers for MSVC to speed up compiling. #200

Closed
wants to merge 1 commit into from

Conversation

gongminmin
Copy link
Contributor

Appveyor building time drops from 9.5 mins to 7.5 mins. On my PC, the time of building stelMain reduces from 1 min to 35 secs.

I also have some code to enable PCH in GCC/Clang. Unfortunately, it has some problem with including Qt headers. So I didn't put that code into this pull request.

@alex-w alex-w requested a review from guillaumechereau June 18, 2018 03:15
@guillaumechereau
Copy link
Contributor

I am not sure I really like that. While I agree that the compile time is too long, I think the proper solution would be to avoid dependencies between the codes by moving some of the algos into separate files (even eventually in C for extra compilation speed).

This patch also further separates the linux build from the Windows build, which is not a good thing in my opinion.

@gzotti
Copy link
Member

gzotti commented Jun 18, 2018

Are precompiled headers a compilation speedup or do they something that changes the end result? On Linux cmake detects ccache to speedup compilation, so I would not oppose if this does not break other things. Maybe make it a cmake option?

@gongminmin
Copy link
Contributor Author

@gzotti PCH doesn't change the end result, it just speed up compiling by pre compile common used headers. If those headers are included by different .cpp, they'll be compiled over and over again. Instead, if they are put into a PCH, they get compiled only once, and then reference that compiled binary.

@guillaumechereau Understand your concern. Yes, it's creating a separation between msvc and other compilers. However, using PCHs in msvc and gcc/clang have different compiler options, it has to be different. Avoid dependencies is not conflict with PCH, it can be benefited by PCH as well.

@alex-w alex-w requested a review from xalioth October 11, 2018 04:30
@375gnu
Copy link
Contributor

375gnu commented Nov 10, 2018

Why not to use https://github.com/sakra/cotire? It's a crossplatform precompiled headers generator for CMake.

@gzotti
Copy link
Member

gzotti commented Nov 10, 2018

That looks interesting!

@gongminmin
Copy link
Contributor Author

I'll try cotire. Thanks @375gnu .

@gongminmin
Copy link
Contributor Author

Just tried cotire, run into this problem...
sakra/cotire#162

@alex-w alex-w changed the title Adds precompiled headers for MSVC to speed up compiling. WIP: Adds precompiled headers for MSVC to speed up compiling. Feb 10, 2019
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the has conflicts The pull request has conflicts label Nov 15, 2020
@alex-w alex-w marked this pull request as draft November 15, 2020 14:36
@gzotti
Copy link
Member

gzotti commented Oct 8, 2021

According to https://github.com/sakra/cotire cotire has been superseded by functionality built into cmake 3.16. Any volunteers to bring this forward? (Note that this must remain optional as long as we don't ask for cmake 3.16+)

@gzotti
Copy link
Member

gzotti commented Mar 10, 2022

@gongminmin have you tried the cmake way to PCH? Else IMHO we should close this PR.

@gzotti
Copy link
Member

gzotti commented Aug 17, 2022

@gongminmin any news?

@gzotti
Copy link
Member

gzotti commented Sep 19, 2022

I close this. OP is unresponsive, and CMake options can be developed in a new PR.

@gzotti gzotti closed this Sep 19, 2022
@gzotti gzotti mentioned this pull request Nov 16, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has conflicts The pull request has conflicts
Development

Successfully merging this pull request may close these issues.

4 participants